DM-55432 - #289
Conversation
b31cab5 to
e9a1350
Compare
isullivan
left a comment
isullivan
left a comment
There was a problem hiding this comment.
A few major issues to highlight:
- check the intended indentation at a few places I point out. I think there are important lines of code that are only executed in one branch of
if/elsestatements, and they should probably apply to either branch. - The delta_ra factors need a
cos(dec)factor in order to describe circular distributions on the sky. - I am worried that columns of the Pandas dataframe are being turned into masked arrays, and later coerced to the correct data type. If so, this could give undefined values in the final catalog for those columns,
| blended_fakes["injection_id"] = self._make_unique_injection_ids( | ||
| len(blended_fakes), | ||
| used_ids=catalog["injection_id"], | ||
| ) | ||
| catalog = vstack([catalog, blended_fakes]) |
There was a problem hiding this comment.
These lines need to be de-indented! If fracHostedBlendedFakes=0, then non-hosted blended fakes are created, but never added to catalog.
| blended_fakes["host_ra"] = blended_fakes["ra"] | ||
| blended_fakes["host_dec"] = blended_fakes["dec"] | ||
|
|
||
| blended_fakes["ra"] += blended_fakes["delta_ra"] / 3600.0 |
There was a problem hiding this comment.
I think this needs a cos(dec) factor.
| rng, n_star_hosted_blended_fakes | ||
| ) | ||
|
|
||
| ra_ssi = ra_hosts + delta_ra / 3600.0 |
There was a problem hiding this comment.
I think this delta_ra also needs a cos(dec) factor.
| variable_fakes["isVariable"] = True | ||
|
|
||
| catalog = vstack([catalog, variable_fakes]) | ||
| catalog["isBlended"] = False |
There was a problem hiding this comment.
This syntax is used in a lot of other places, but since catalog is Pandas I am worried that setting = False will turn this column into a masked array. Please double-check the type of the column at this point in execution (just a print statement in a test should do).
| if len(catalog) > len(np.unique(catalog["injection_id"])): | ||
| self.log.warning("Duplicate injection IDs detected after catalog assembly; reassigning them.") | ||
| old_injection_ids = np.asarray(catalog["injection_id"], dtype=np.int64) | ||
| new_injection_ids = self._make_unique_injection_ids(len(catalog)) | ||
| # re-assign fresh injection ids | ||
| catalog["injection_id"] = new_injection_ids | ||
| if "twin_id" in catalog.colnames: | ||
| id_map = {old_id: new_id for old_id, new_id in zip(old_injection_ids, new_injection_ids)} | ||
| catalog["twin_id"] = np.asarray( | ||
| [id_map.get(int(twin_id), int(twin_id)) for twin_id in catalog["twin_id"]], | ||
| dtype=np.int64, | ||
| ) |
There was a problem hiding this comment.
I think this block is also indented one level too far, since it is now inside if self.config.doAddBlendedFakes but I think it could be run in either case.
| catalog["host_ra"] = catalog["host_ra"].value | ||
| catalog["host_dec"] = catalog["host_dec"].value |
There was a problem hiding this comment.
This might need an additional check - I don't think it is guaranteed that host_ra and host_dec are present at this point.
| flagCut &= ~sourceCat['base_PixelFlags_flag'] | ||
|
|
||
| extendednessCut = sourceCat['base_ClassificationSizeExtendedness_value'] < 0.9 | ||
| extendednessCut &= sourceCat['base_ClassificationExtendedness_value'] != 1 |
There was a problem hiding this comment.
I think you want extendednessCut &= sourceCat['base_ClassificationExtendedness_value'] == 0 here. != 1 would accept corrupt data, such as NaN.
| return 0 | ||
|
|
||
| cap = n_hosts * self.config.maxHostedBlendedFakesPerHost | ||
| if self.config.maxHostedBlendedFakesTotal > 0: |
There was a problem hiding this comment.
An edge case, but if someone set self.config.maxHostedBlendedFakesTotal=0 intending to use that to disable adding hosted blended fakes, it would actually do the opposite and disable the check and cap.
| if self.config.fracHostedBlendedFakes > 0: | ||
| hostcatalog = photoCalib.calibrateCatalog(sourceCat).asAstropy() | ||
| star_hosts = self.select_host_stars(hostcatalog) | ||
| # if len(star_hosts) is less than the blended fakes, then use replacement |
There was a problem hiding this comment.
The comment says "... then use replacement", but I don't see that happening. Is the comment from an older version? Also, note that if the number of hosts is capped, then the total number of blended fakes will be effectively reduced (below fracBlendedFakes).
| ('run', '<U62'), | ||
| ('band', '<U1') |
There was a problem hiding this comment.
run and band are added to the schema, but I don't see either set anywhere.
2 participants
Add optional blended fakes to catalog creation tool in AP